Fix log redaction, device registration race, and incomplete reset - #12
Closed
mohn93 wants to merge 1 commit into
Closed
Fix log redaction, device registration race, and incomplete reset#12mohn93 wants to merge 1 commit into
mohn93 wants to merge 1 commit into
Conversation
Fixes #9, #10 and #11. The three touch overlapping files, so they land together. #9 - credentials and PII in the logs (enableLogging: true only) Three sites wrote secrets verbatim: device_service.dart logged Device.toJson(), which carries the full FCM token; subscriber_service logged the login payload - name, email, phone, metadata; tag_service logged tag values, which routinely hold an email, plan or region. Every request body also reached logApiRequest verbatim, repeating the same values at debug level. Adds PushFireLogger.redact, applied to device info and to request and response bodies. The token keeps the existing first/last-ten mask so it stays correlatable with the server; everything else becomes <redacted>. Identifiers - externalId, deviceId, subscriberId, tagId - are left alone: they are what a support ticket is traced by. Response bodies are redacted too, since register-device returns the device row and login-subscriber the subscriber row; a body that is not a JSON object passes through unchanged. #10 - registerDevice had no concurrency guard registerDevice reads the stored device id, makes a network round trip, then writes the id back. Two callers entering before either wrote both saw no id, both POSTed, and created two device rows for one device. The second write won locally, so the first row was orphaned server-side while still holding the same FCM token. Reachable from auto-registration at init, the token-refresh handler, the foreground permission check and requestNotificationPermission. registerDevice is now single-flight: concurrent callers join the in-flight registration. The guard is released on failure too, so a failed attempt does not wedge later callers. The permission check is coalesced the same way. The old _isCheckingPermission flag guarded only the resume path, so a token refresh arriving during a resume ran two overlapping checks; both syncNotificationPermission and the token-refresh handler now go through it. Callers that join receive null, so one permission change still emits exactly one onDeviceRegistered. #11 - reset() could leave state behind Two failure modes. A stored subscriber blob whose id is null does not count as logged in, so the gated logout skipped it and the blob - name, email, phone - survived a call documented as clearing all local state; on a shared device the next user's session started holding the previous user's details. And logoutSubscriber clears locally then rethrows, so a failed logout request propagated out of reset() and clearDeviceData() never ran, leaving the device id, FCM token and permission state behind. Both clears are now unconditional and run after a logout that cannot escape. clearSubscriberData is public for that; the teardown is extracted as clearAllLocalState so it can be tested without a live SDK singleton, which needs Firebase. 28 regression tests. Each was checked to fail with its fix reverted.
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9, closes #10, closes #11.
Stacked on #8 — the three fixes touch
device_service.dartandpushfire_sdk_impl.dart, which #8 also changes. Base retargets tomainautomatically once #8 merges.
#9 — credentials and PII in the logs
Applies only when
enableLogging: true, so a debug-mode leak rather than adefault one.
Three sites wrote secrets verbatim:
device_service.dart:76loggedDevice.toJson(), which carries the full FCMtoken. An FCM token is a send capability, and device logs are read by other
tooling and swept up by crash collectors.
subscriber_service.dart:50logged the login payload: name, email, phone andthe whole metadata map.
tag_service.dart:17and:62logged tag values, which routinely hold anemail, plan or region.
Every request body also reached
logApiRequestverbatim, repeating the samevalues at debug level.
Adds
PushFireLogger.redact, applied to device info and to request and responsebodies. The token keeps the existing first/last-ten mask so it stays
correlatable with the server; everything else becomes
<redacted>.Identifiers —
externalId,deviceId,subscriberId,tagId— are leftalone: they are what a support ticket is traced by.
Beyond what the issue asked: response bodies are redacted too.
register-devicereturns the device row and
login-subscriberthe subscriber row, so redactingonly requests would have reopened the same leak from the other direction. A body
that is not a JSON object — an HTML gateway page — passes through unchanged.
The API key was never affected; the Authorization header is not passed to a
logger.
#10 — registerDevice had no concurrency guard
registerDevicereads the stored device id, makes a network round trip, thenwrites the id back. Two callers entering before either wrote both saw no id,
both POSTed, and created two device rows for one device. The second write won
locally, so the first row was orphaned server-side while still holding the same
FCM token.
registerDeviceis now single-flight: concurrent callers join the in-flightregistration. The guard is released on failure too, so a failed attempt — a cold
iOS start before APNS answers — does not wedge every later caller.
The permission check is coalesced the same way. The old
_isCheckingPermissionflag guarded only the resume path, so a token refresh arriving during a resume
ran two overlapping checks. Both
syncNotificationPermissionand thetoken-refresh handler now go through the shared guard. Callers that join receive
null rather than being dropped, so one permission change still emits exactly one
onDeviceRegistered.#11 — reset() could leave state behind
Two failure modes:
idis null does not count as logged in, sothe gated logout skipped it and the blob — name, email, phone — survived a
call documented as clearing all local state. On a shared device the next
user's session started holding the previous user's details.
logoutSubscriberclears locally then rethrows, so a failed logout requestpropagated out of
reset()andclearDeviceData()never ran, leaving thedevice id, FCM token, permission status and preference behind.
Both clears are now unconditional and run after a logout that cannot escape.
clearSubscriberDatais public for that. The teardown is extracted asclearAllLocalStateso it can be tested without a live SDK singleton, whichneeds Firebase.
Tests
419 pass, up from 391. Each of the 28 new tests was checked to fail with its own
fix reverted:
test/utils/logger_redaction_test.dart— the redaction helpers, plus theemitted log records for
logDeviceInfo,logApiRequestandlogApiResponse.test/services/service_logging_pii_test.dart— whatloginSubscriber,addTagandupdateTagactually write while doing real work.test/services/device_service_concurrent_registration_test.dart— 2 and 5concurrent callers produce one POST; a rotated token still reaches the server;
a failed registration releases the guard; joiners see the failure.
test/pushfire_sdk_reset_test.dart— the null-id blob, the failed logout, anunexpected throw, and a clean install.
No parity divergence introduced: the native Swift SDK marks bodies
privacy: .private, coalesces registration and the permission check, and clearsboth stores unconditionally after a
try?logout.